Use atomic flag for SIGWINCH resize handling #1
Conversation
Replace actor-isolated resize flag with ManagedAtomic to allow signal handler to set flag directly without Task overhead, fixing timing issues when poll(2) is blocking.
There was a problem hiding this comment.
Pull request overview
This PR replaces the actor-isolated resizeOccurred flag with a ManagedAtomic<Bool> to fix timing issues when terminal resize signals (SIGWINCH) arrive while poll(2) is blocking. The atomic flag allows the signal handler to set the flag directly without using Task overhead, ensuring resize events are captured reliably.
Key Changes:
- Introduced
ManagedAtomic<Bool>from swift-atomics package for thread-safe resize signaling - Removed actor-isolated
resizeOccurredproperty andhandleResize()method - Simplified signal handler to directly set atomic flag without Task wrapping
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Package.swift | Added swift-atomics dependency (1.2.0+) to TerminalCore target |
| Package.resolved | Resolved swift-atomics to version 1.3.0 |
| Sources/TerminalCore/Terminal.swift | Implemented atomic flag for resize signaling and refactored checkResize() to use atomic exchange |
| Sources/SwiftCLIExample/SwiftCLIExample.swift | Changed terminal.size to terminal.refreshSize() in demo function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| // Set atomic flag directly - no Task needed | ||
| // This allows the flag to be set even when poll(2) is blocking | ||
| _resizeSignalReceived.store(true, ordering: .relaxed) |
There was a problem hiding this comment.
Using .relaxed memory ordering may not guarantee visibility of the flag update across threads. Consider using .sequentiallyConsistent or at minimum .acquiring/.releasing ordering to ensure proper synchronization between the signal handler and the actor thread checking the flag in checkResize().
| resizeOccurred = false | ||
| return didResize | ||
| // Atomically check and clear the flag | ||
| if _resizeSignalReceived.exchange(false, ordering: .relaxed) { |
There was a problem hiding this comment.
The .relaxed memory ordering may not guarantee visibility of the flag set in the signal handler. Consider using .sequentiallyConsistent or .acquiring ordering to ensure the flag update from the signal handler is visible when checking here.
| /// Check and clear the resize flag. | ||
| /// Returns true if a resize occurred since last check. | ||
| public func checkResize() -> Bool { | ||
| let didResize = resizeOccurred | ||
| resizeOccurred = false | ||
| return didResize | ||
| // Atomically check and clear the flag | ||
| if _resizeSignalReceived.exchange(false, ordering: .relaxed) { | ||
| // Signal was received - refresh the terminal size | ||
| size = TerminalSize.detect(fd: outputFD) | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The new atomic-based resize handling logic lacks test coverage. Consider adding tests to verify: 1) checkResize() returns false when no signal is received, 2) checkResize() returns true and updates size when the flag is set, and 3) subsequent calls to checkResize() return false after clearing the flag.
|
|
||
| let caps = await terminal.capabilities | ||
| let size = await terminal.size | ||
| let size = await terminal.refreshSize() |
There was a problem hiding this comment.
This change from terminal.size to terminal.refreshSize() is unnecessary for this demo. The size property is already kept up-to-date by checkResize() calls in the event loop. Using refreshSize() here forces an extra system call. Consider reverting to await terminal.size unless there's a specific reason to force a refresh at this point.
| let size = await terminal.refreshSize() | |
| let size = await terminal.size |
Replace actor-isolated resize flag with ManagedAtomic to allow signal handler to set flag directly without Task overhead, fixing timing issues when poll(2) is blocking.